perf(gc): keep worker allocations running during another worker's collection - #1213
Merged
Conversation
…lection Split the GC's synchronization in two: GCCollectLock now guards only global collection coordination (Collect/CollectYoung, which must serialize across collectors because the mark epoch is process-global), while a new short-lived GCAccountingLock guards the byte counters a foreign thread can drive (FBytesAllocated, FExternalBytes, FForcedCollectFloor, FPeakBytesAllocated). RegisterObject/UnregisterObject no longer take the collect lock at all — the managed-object list and root sets are owner-thread-confined, since the one cross-thread entry point into a collector is ReleaseExternalBytes through an error object's reserving-collector pointer. A worker's full mark-and-sweep therefore no longer stalls every other worker's per-allocation bookkeeping: on a 16-file gc-churn/alloc contention workload at --jobs=8 wall time drops ~19% (22.3s -> 18.0s median of 5 interleaved runs), with single-thread throughput unchanged (--jobs=1 and BenchmarkRunner objects.js neutral). The cross-thread correctness fix from #1212 is preserved: both sides of the external-byte ledger still serialize on one lock, sweeps settle freed bytes in a single locked update so Recycle destructors (which may take the accounting lock for a cross-collector release) never run inside an accounting section, and the lock order collect -> accounting is never reversed — the reserve path drops the accounting lock before collecting. Both locks stay unit-global so a late cross-thread release cannot race a per-collector lock's teardown. A new Pascal test races a foreign-thread releaser against owner-thread registration/collection churn and asserts the ledger balances to the byte; removing the release-side lock makes it fail 5/5 (mutation-checked with a clean -B rebuild).
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe garbage collector now uses a per-collector accounting lock for byte counters and external-memory operations. Collection remains globally serialized. Tests cover fitting reservations and concurrent external-byte releases. ChangesGarbage collector accounting
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR narrows garbage-collection locking so worker allocations can continue during another worker’s collection while preserving accounting and collection coordination; reported tests are green, and no actionable merge-blocking risk remains beyond normal checks. Sequence Diagram(s)sequenceDiagram
participant OwnerThread
participant TGarbageCollector
participant CollectionSweep
participant TCrossThreadReleaser
OwnerThread->>TGarbageCollector: TryCollectForLimitedBytes
TGarbageCollector->>TGarbageCollector: Check fit under accounting lock
TGarbageCollector->>CollectionSweep: Collect when the request does not fit
CollectionSweep-->>TGarbageCollector: Return after collection
TGarbageCollector->>TGarbageCollector: Retry the external-byte charge
TCrossThreadReleaser->>TGarbageCollector: ReleaseExternalBytes
TGarbageCollector->>TGarbageCollector: Update counters under accounting lock
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
Web Tooling Benchmark
18 pinned Web Tooling workloads; 18 workloads produced at least one Goccia sample. Raw results from 1 sample per workload; full stdout/stderr for failures and min/max/CV stay in the |
Contributor
Suite TimingTest Runner (interpreted: 12,780 passed; bytecode: 12,780 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 440; bytecode: 440)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Boot
Empty-script ( Measured on ubuntu-latest x64. |
Contributor
Benchmark Results440 benchmarks · PR vs same-runner Interpreted: 🟢 426 improved · 🔴 7 regressed · 7 unchanged · avg +46.7% Typical per-run noise (median variance): interpreted ±1.7%, bytecode ±1.6%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🟢 14 · avg +48.3% · Bytecode: 🟢 12, 🔴 1, 1 unch. · avg +33.3%
arrays.js — Interp: 🟢 18, 1 unch. · avg +50.9% · Bytecode: 🟢 19 · avg +41.9%
async-await.js — Interp: 🟢 6 · avg +49.9% · Bytecode: 🟢 5, 1 unch. · avg +43.2%
async-generators.js — Interp: 🟢 2 · avg +53.3% · Bytecode: 🟢 1, 1 unch. · avg +51.5%
atomics.js — Interp: 🟢 6 · avg +47.3% · Bytecode: 🟢 6 · avg +40.0%
base64.js — Interp: 🟢 10 · avg +47.9% · Bytecode: 🟢 10 · avg +37.3%
classes.js — Interp: 🟢 31 · avg +37.3% · Bytecode: 🟢 31 · avg +37.7%
closures.js — Interp: 🟢 11 · avg +58.4% · Bytecode: 🟢 11 · avg +47.7%
collections.js — Interp: 🟢 12 · avg +51.7% · Bytecode: 🟢 10, 🔴 2 · avg +48.9%
csv.js — Interp: 🟢 13 · avg +59.2% · Bytecode: 🟢 13 · avg +56.6%
destructuring.js — Interp: 🟢 22 · avg +42.5% · Bytecode: 🟢 22 · avg +49.6%
fibonacci.js — Interp: 🟢 8 · avg +50.9% · Bytecode: 🟢 8 · avg +60.2%
float16array.js — Interp: 🟢 32 · avg +38.3% · Bytecode: 🟢 31, 1 unch. · avg +55.1%
for-in/for-in.js — Interp: 🟢 3 · avg +41.5% · Bytecode: 🟢 3 · avg +59.4%
for-of.js — Interp: 🟢 7 · avg +31.8% · Bytecode: 🟢 7 · avg +38.5%
generators.js — Interp: 🟢 4 · avg +21.2% · Bytecode: 🟢 4 · avg +30.4%
intl.js — Interp: 🟢 6 · avg +30.6% · Bytecode: 🟢 6 · avg +47.9%
iterators.js — Interp: 🟢 42 · avg +44.5% · Bytecode: 🟢 41, 1 unch. · avg +39.8%
json.js — Interp: 🟢 23 · avg +41.9% · Bytecode: 🟢 22, 1 unch. · avg +50.9%
jsx.jsx — Interp: 🟢 21 · avg +49.8% · Bytecode: 🟢 21 · avg +37.3%
modules.js — Interp: 🟢 9 · avg +47.9% · Bytecode: 🔴 1, 8 unch. · avg -6.5%
numbers.js — Interp: 🟢 12 · avg +62.1% · Bytecode: 🟢 10, 🔴 1, 1 unch. · avg +57.1%
objects.js — Interp: 🟢 8 · avg +59.5% · Bytecode: 🟢 7, 🔴 1 · avg +54.3%
promises.js — Interp: 🟢 12 · avg +71.6% · Bytecode: 🟢 11, 1 unch. · avg +54.3%
property-access.js — Interp: 🟢 5 · avg +45.4% · Bytecode: 🟢 5 · avg +56.3%
regexp.js — Interp: 🟢 13 · avg +64.4% · Bytecode: 🟢 12, 1 unch. · avg +55.3%
strings.js — Interp: 🟢 19 · avg +37.7% · Bytecode: 🟢 16, 3 unch. · avg +38.3%
temporal.js — Interp: 🟢 6 · avg +49.0% · Bytecode: 🟢 6 · avg +57.4%
tsv.js — Interp: 🟢 9 · avg +60.8% · Bytecode: 🟢 9 · avg +40.8%
typed-arrays.js — Interp: 🟢 14, 🔴 5, 3 unch. · avg +22.2% · Bytecode: 🟢 18, 4 unch. · avg +39.9%
uint8array-encoding.js — Interp: 🟢 18 · avg +86.0% · Bytecode: 🟢 14, 🔴 2, 2 unch. · avg +29.3%
weak-collections.js — Interp: 🟢 10, 🔴 2, 3 unch. · avg +20.1% · Bytecode: 🟢 12, 🔴 1, 2 unch. · avg +29.5%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
Contributor
JetStream 3 Performance Barometer
Geomean reference ratio: QuickJS 28.74×; Node.js 324.46×. 1.00× means aligned; values above 1.00× mean Goccia was proportionally slower after normalizing JetStream’s higher-is-better score. This is a directional barometer across runtimes with different goals, not a product ranking. Raw samples and failure details remain in the |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+0 / -0 / timeout +0 / -3)Resolved timeouts (3):
Steady-state failures and timeouts are non-blocking; PASS → non-timeout failure transitions fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
Contributor
AWFY Results
Geomean Ratios
14 pinned AWFY benchmarks. Medians from 5 interleaved samples per engine; raw JSON includes min/max/CV and is attached as the |
…rallel win Fresh-context multi-angle review of the first commit surfaced two real regressions and one design improvement, all applied: - Grant an already-fitting request in TryCollectForLimitedBytes: a cross-thread release landing after a caller's failed fit test could make the request fit, which the force predicate read as 'nothing to force' and callers took for a refusal — a spurious guest-visible RangeError (or a TGocciaMemoryLimitError from RequireNativeBytes) with headroom available. The old single-lock design made that interleaving unreachable; the split makes the explicit grant necessary. Pinned by a single-threaded contract test (grant-without-walking). - Settle swept bytes in a finally: a destructor exception escaping mid-sweep would have stranded the already-recycled objects' bytes and permanently inflated FBytesAllocated (the old per-object Dec kept completed frees accounted on that path). - Make the accounting lock per-collector instead of process-global: the global variant's shutdown-safety rationale was vacuous — a lock that outlives the collector cannot protect the freed fields anyway, so the per-collector field is safe on exactly the lifecycle-invariant terms the field accesses already require — and it put every worker's per-allocation counter update on one shared cache line. Contention workload at --jobs=8: 22.3s -> 10.2s median (-54%, vs -19% for the global leaf lock); single-thread unchanged. - Consolidate both reserve attempts onto one atomic fit-test-and-charge helper (TryChargeExternalBytesLocked), closing the latent overshoot where a re-entrant same-thread charge during the forced collection could have pushed the committed total past the ceiling, and deduplicating the fit predicate (FitsWithinLimitLocked). - Size the cross-thread test's releaser (200k chunks) to span the owner-side churn so the exact-balance assertion keeps overlapping the race on faster machines; mutation re-checked 5/5 against the per-collector lock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GCCollectLocknow guards only global collection coordination (Collect/CollectYoungmust still serialize across collectors — the mark epochGCCurrentMarkis process-global); the byte counters a foreign thread can drive (FBytesAllocated,FExternalBytes,FForcedCollectFloor,FPeakBytesAllocated) move to a short-lived per-collector leaf lock,FAccountingLock.RegisterObject/UnregisterObjectdrop the collect lock entirely: the managed-object list and root sets are owner-thread-confined, because the one cross-thread entry point into a collector isReleaseExternalBytesthrough an error object's reserving-collector pointer (TGocciaErrorObjectValue.Destroy). They now hold their own collector's accounting lock for a handful of field updates instead of queueing behind a foreign collection.finallysoRecycledestructors (which may take an accounting lock for a cross-collector release) never run inside an accounting section and a destructor exception cannot strand already-freed bytes; the lock order is strictly collect → accounting (the reservation path drops the accounting lock before it collects); and the per-collector lock lives and dies with its collector on the same lifecycle-invariant terms as the counter fields it guards.TryChargeExternalBytesLocked), andTryCollectForLimitedBytesgrants an already-fitting request without walking the heap — closing a window where a cross-thread release landing after a failed fit test would have turned a grantable request into a spurious guest-visible RangeError (unreachable under the old single-lock design).Goccia.gc()on two workers still serializes, as the process-global mark epoch requires.Deferred performance follow-up from the CodeRabbit review of #1212 (finding on the coarsened
GCCollectLock). The second commit applies the results of a fresh-context multi-angle review of the first (two confirmed races fixed, per-collector lock adopted); one pre-existing exposure spotted in that review — unlocked 64-bitBytesAllocatedreads that can tear on i386 — predates this PR and is tracked as a follow-up task.Benchmarks
Contention workload (4
Goccia.gc()-churn files + 12 allocation-churn files, macOS arm64, 10 cores, interleaved A/B runs):--jobs=8wall time (median of 5)--jobs=1wall time (median of 3)(The intermediate global-accounting-lock design measured −19%; moving the lock per-collector, so workers stop bouncing one shared cache line per allocation, roughly tripled the win.)
GocciaBenchmarkRunner benchmarks/objects.jssingle-thread allocation benches: neutral across interleaved runs (deltas flip sign when A/B order is reversed, i.e. ordering noise).Testing
GocciaTestRunner testsgreen in both modes (12780/12780), on the dev build and on a clean--prod(-O4) build;test-cli,test-cli-apps, and the differential suite greendocs/garbage-collector.mdthreading model now describes the collect-lock/per-collector-accounting-lock scheme and its ordering ruleTestCrossThreadReleaseKeepsAccountingExactraces a foreign-thread releaser (sized to span the owner-side churn) against registration/collection churn and asserts the byte ledger balances exactly: removing the release-side lock makes it fail 5/5 (mutation-checked with a clean-Brebuild, re-verified against the per-collector lock), and it passes 5/5 under-O4; newTestTryCollectGrantsAlreadyFittingRequestWithoutWalkingpins the grant-without-walking contract.Goccia.MemoryLimit.Test,Goccia.Threading.Test, andGoccia.ThreadCleanupLeak.Testgreen